Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add ECS and Logs attributes based on Metadata v4 endpoint #875

Merged
merged 3 commits into from
Mar 13, 2023
Merged

Add ECS and Logs attributes based on Metadata v4 endpoint #875

merged 3 commits into from
Mar 13, 2023

Conversation

mmanciop
Copy link
Contributor

@mmanciop mmanciop commented Jan 9, 2023

Changes

Implement aws.ecs and aws.log resource attributes with data from the ECS Metadata endpoint v4.

  • Appropriate CHANGELOG.md updated for non-trivial changes

@mmanciop mmanciop requested a review from a team January 9, 2023 12:39
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jan 9, 2023 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@mmanciop mmanciop marked this pull request as draft January 9, 2023 12:39
@utpilla
Copy link
Contributor

utpilla commented Jan 11, 2023

@mmanciop Please sign the CLA.

@utpilla utpilla added the comp:extensions.aws Things related to OpenTelemetry.Extensions.AWS label Jan 11, 2023
@mmanciop mmanciop marked this pull request as ready for review January 11, 2023 12:18
@mmanciop mmanciop force-pushed the ecs_detector branch 3 times, most recently from e7430ae to 49ac4d3 Compare January 11, 2023 14:53
@mmanciop
Copy link
Contributor Author

mmanciop commented Jan 11, 2023 •
edited
Loading

I am actually not 100% sure yet of the patchset wrt the implementation of the IResourceDetector interface. I am going to try it out on a test app.

Update: It seems to me we need to get rid of the local IResourceDetector if we want this to work with OpenTelemetry SDK 1.3.0+.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jan 11, 2023 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@mmanciop mmanciop force-pushed the ecs_detector branch 3 times, most recently from 3c5403f to 4d189d8 Compare January 11, 2023 22:16
@mmanciop mmanciop requested a review from Kielek January 11, 2023 22:22
@Kielek Kielek requested a review from srprash January 13, 2023 06:43
@Kielek
Copy link
Contributor

Kielek commented Jan 13, 2023

@srprash, @lupengamzn could you please check?

@github-actions github-actions bot added the Stale label Feb 14, 2023
@github-actions
Copy link
Contributor

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Feb 22, 2023
@Kielek Kielek reopened this Feb 22, 2023
@Kielek Kielek removed the Stale label Feb 22, 2023
@Kielek
Copy link
Contributor

Kielek commented Feb 22, 2023

@srprash?

@github-actions
Copy link
Contributor

github-actions bot commented Mar 2, 2023

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Mar 2, 2023
@srprash
Copy link
Contributor

srprash commented Mar 2, 2023

@srprash, @lupengamzn could you please check?

Would you review? And also comment on the breaking change/major version bump as well?

I agree that this must go out as a major version bump.

Just a reminder, in case of major bump, the whole package should be renamed. At least .Contrib infix has to be removed.

Yup. We can do that to be inline with the rest of the packages in the repo. I wish we can do the same for the OpenTelemetry.Contrib.Instrumentation.AWS package as well while bumping its major version.

If renaming the package is being considered, could we please replace XRay with AWS in the name?

I have no issues in dropping XRay and renaming the package to be OpenTelemetry.Contrib.Extensions.AWS. We had it with XRay because initially the package contained only the id-generator and the propagator very specific to AWS XRay.

@github-actions github-actions bot removed the Stale label Mar 3, 2023
@Kielek
Copy link
Contributor

Kielek commented Mar 3, 2023

@mmanciop, finally we have an approval from the package owner.
Could you please solve the conflicts? Then we should be able to make final review and merge.

@mmanciop
Copy link
Contributor Author

mmanciop commented Mar 9, 2023 •
edited
Loading

@Kielek @srprash I solved the conflicts and did some cleanup, but I am assuming that version bumping, extension renaming and the rest mentioned in #875 (comment) will be performed by the package owners, right?

@Kielek
Copy link
Contributor

Kielek commented Mar 9, 2023

For sure as a separate PR.

@Kielek Kielek mentioned this pull request Mar 9, 2023
Closed
@mmanciop
Copy link
Contributor Author

@mmanciop, finally we have an approval from the package owner. Could you please solve the conflicts? Then we should be able to make final review and merge.

Now that I found out why PublicApiAnalyzers did not run in my setup (#1071), I dearly hope this commit is it :-)

@Kielek
fix: more public API fixes
44e675a 
Co-authored-by: Piotr Kiełkowicz <[email protected]>
@utpilla utpilla merged commit 5f33458 into open-telemetry:main Mar 13, 2023
@mmanciop mmanciop deleted the ecs_detector branch March 13, 2023 17:57
@utpilla utpilla mentioned this pull request Mar 14, 2023
Merged
@Kielek Kielek mentioned this pull request Jun 9, 2023
Merged
@rypdal rypdal mentioned this pull request Jun 9, 2023
Merged
@Kielek Kielek mentioned this pull request Jul 27, 2023
Merged
@mmanciop mmanciop mentioned this pull request Jan 31, 2024
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atshaw43 atshaw43 atshaw43 approved these changes

@srprash srprash srprash approved these changes

@Kielek Kielek Kielek approved these changes

Assignees

@hananiel hananiel

@cijothomas cijothomas

@SergeyKanzhelev SergeyKanzhelev

@mic-max mic-max

@Yun-Ting Yun-Ting

@reyang reyang

@CodeBlanch CodeBlanch

@srprash srprash

@utpilla utpilla

Labels
comp:extensions.aws Things related to OpenTelemetry.Extensions.AWS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.